perf(timeline): memoize VList timeline items to prevent mass re-renders#660
Closed
Just-Insane wants to merge 35 commits intoSableClient:devfrom
Closed
perf(timeline): memoize VList timeline items to prevent mass re-renders#660Just-Insane wants to merge 35 commits intoSableClient:devfrom
Just-Insane wants to merge 35 commits intoSableClient:devfrom
Conversation
Contributor
Author
|
Added a follow-up fix in the latest commit: |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR focuses on reducing unnecessary timeline re-renders and improving perceived performance when navigating within/between rooms by memoizing per-item rendering and introducing a session-scoped VList scroll/measurement cache.
Changes:
- Memoize individual
VListtimeline rows viaReact.memoand add stable-reference reuse inuseProcessedTimelineto reduce re-renders. - Track timeline “content mutation” separately from event arrivals via a new
mutationVersioninuseTimelineSync, and plumb it intouseProcessedTimeline. - Add a per-room (in-memory) scroll + VList cache snapshot and restore it on revisit to skip the initial stabilization delay; refine notification jumping behavior to avoid unnecessary historical timeline loads.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/utils/roomScrollCache.ts | Adds an in-memory per-room scroll/VList cache snapshot store. |
| src/app/utils/roomScrollCache.test.ts | Unit tests for the new scroll cache utility. |
| src/app/hooks/useNotificationJumper.ts | Avoids navigating via eventId when the target event is already in the live timeline; retries when live timeline is initially empty. |
| src/app/hooks/timeline/useTimelineSync.ts | Introduces mutationVersion and a shared mutation trigger to distinguish content mutations from new-event arrivals. |
| src/app/hooks/timeline/useProcessedTimeline.ts | Adds stable-ref caching and filters poll response/end events from processed output. |
| src/app/hooks/timeline/useProcessedTimeline.test.ts | Adds coverage for stable-ref reuse behavior and poll-response filtering. |
| src/app/features/room/RoomTimeline.tsx | Wraps timeline item rendering in a memoized component; adds scroll/cache restore + persistence and programmatic-scroll guarding. |
| .changeset/perf-timeline-scroll-cache.md | Changeset entry for scroll-cache optimization. |
| .changeset/perf-timeline-item-memo.md | Changeset entry for item memoization optimization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [setAtBottom, room.roomId, eventId] | ||
| ); | ||
|
|
||
| const showLoadingPlaceholders = |
There was a problem hiding this comment.
showLoadingPlaceholders no longer checks timelineSync.eventsLength === 0, so for rooms where canPaginateBack is true it will render placeholders for every row and hide real timeline items. Align the condition with vListItemCount (only show placeholders when there are zero events and we're still loading/back-paginating the initial view).
Suggested change
| const showLoadingPlaceholders = | |
| const showLoadingPlaceholders = | |
| timelineSync.eventsLength === 0 && |
Comment on lines
+208
to
+222
| // Reuse the previous ProcessedEvent object if all structural fields match, | ||
| // so that React.memo on timeline item components can bail out cheaply. | ||
| // itemIndex must also be equal: after back-pagination the same eventId | ||
| // shifts to a higher VList index, so a stale itemIndex would break | ||
| // getRawIndexToProcessedIndex and focus-highlight comparisons. | ||
| const prev = prevCache?.get(mEventId); | ||
| const stable = | ||
| prev && | ||
| prev.itemIndex === processed.itemIndex && | ||
| prev.collapsed === collapsed && | ||
| prev.willRenderNewDivider === willRenderNewDivider && | ||
| prev.willRenderDayDivider === willRenderDayDivider && | ||
| prev.eventSender === eventSender | ||
| ? prev | ||
| : processed; |
There was a problem hiding this comment.
The stable-ref reuse logic only compares a few derived fields (itemIndex/collapsed/dividers/sender) and can return a cached ProcessedEvent that holds a stale mEvent/timelineSet when the SDK provides a new MatrixEvent instance for the same eventId (e.g. timeline reset/rebuild). Include prev.mEvent === mEvent and prev.timelineSet === timelineSet (or otherwise ensure the cached object always points at the current event/timelineSet) before reusing the old reference.
Comment on lines
+12
to
+21
| /** Session-scoped, per-room scroll cache. Not persisted across page reloads. */ | ||
| const scrollCacheMap = new Map<string, RoomScrollCache>(); | ||
|
|
||
| export const roomScrollCache = { | ||
| save(roomId: string, data: RoomScrollCache): void { | ||
| scrollCacheMap.set(roomId, data); | ||
| }, | ||
| load(roomId: string): RoomScrollCache | undefined { | ||
| return scrollCacheMap.get(roomId); | ||
| }, |
There was a problem hiding this comment.
roomScrollCache is keyed only by roomId, but the app supports multi-account sessions. This can cause scroll state (and VList cache snapshots) to bleed across accounts that share the same roomId, leading to incorrect scroll restoration. Consider keying by a compound key (e.g. ${userId}:${roomId} or activeSessionId) or exposing session-scoped instances of the cache.
Suggested change
| /** Session-scoped, per-room scroll cache. Not persisted across page reloads. */ | |
| const scrollCacheMap = new Map<string, RoomScrollCache>(); | |
| export const roomScrollCache = { | |
| save(roomId: string, data: RoomScrollCache): void { | |
| scrollCacheMap.set(roomId, data); | |
| }, | |
| load(roomId: string): RoomScrollCache | undefined { | |
| return scrollCacheMap.get(roomId); | |
| }, | |
| export type RoomScrollCacheStore = { | |
| save(roomId: string, data: RoomScrollCache): void; | |
| load(roomId: string): RoomScrollCache | undefined; | |
| }; | |
| /** Session-scoped, per-room scroll cache. Not persisted across page reloads. */ | |
| const scrollCacheMap = new Map<string, RoomScrollCache>(); | |
| function toCacheKey(sessionId: string, roomId: string): string { | |
| return `${sessionId}:${roomId}`; | |
| } | |
| export const roomScrollCache = { | |
| save(sessionId: string, roomId: string, data: RoomScrollCache): void { | |
| scrollCacheMap.set(toCacheKey(sessionId, roomId), data); | |
| }, | |
| load(sessionId: string, roomId: string): RoomScrollCache | undefined { | |
| return scrollCacheMap.get(toCacheKey(sessionId, roomId)); | |
| }, | |
| forSession(sessionId: string): RoomScrollCacheStore { | |
| return { | |
| save(roomId: string, data: RoomScrollCache): void { | |
| roomScrollCache.save(sessionId, roomId, data); | |
| }, | |
| load(roomId: string): RoomScrollCache | undefined { | |
| return roomScrollCache.load(sessionId, roomId); | |
| }, | |
| }; | |
| }, |
| key={room.roomId} | ||
| ref={vListRef} | ||
| data={processedEvents} | ||
| cache={scrollCacheForRoomRef.current?.cache} |
There was a problem hiding this comment.
The cached VList cache snapshot is always passed into VList, even when viewing a historical slice via eventId. Since the cached snapshot was recorded for the live timeline indices, applying it to a sparse eventId timeline can yield incorrect size estimates/scroll positioning. Consider passing cache only when !eventId (or maintaining separate caches for live vs eventId timelines).
Suggested change
| cache={scrollCacheForRoomRef.current?.cache} | |
| cache={!eventId ? scrollCacheForRoomRef.current?.cache : undefined} |
- useTimelineSync: add mutationVersion counter, incremented only on mutations (reactions, edits, local-echo, thread updates) via a new triggerMutation() callback. Live event arrivals do NOT bump it — the eventsLength change already signals those. - useProcessedTimeline: add stableRefsCache (useRef<Map>) that reuses the same ProcessedEvent object across renders when mutationVersion is unchanged and structural fields (collapsed, dividers, eventSender) are identical. New mutationVersion bypasses the cache so fresh objects reach React on actual content mutations. - RoomTimeline: define TimelineItem as React.memo outside the component function so the type is stable. Render via renderFnRef (synchronously updated each cycle) to avoid stale closures without adding to deps. Per-item boolean props (isHighlighted, isEditing, isReplying, isOpenThread) and a settingsEpoch object let memo skip re-renders on unchanged items while still re-rendering the one item that changed. vListIndices deps changed from timelineSync.timeline (always a new object from spread) to timelineSync.timeline.linkedTimelines + timelineSync.mutationVersion. Expected gains: Scrolling: 0 item re-renders (was: all visible items) New message: 1 item re-renders (was: all) focusItem/editId change: 1-2 items (was: all) Reactions/edits/mutations: all items (same as before, content changed) Settings change: all items via settingsEpoch (same as before)
…ptions ThreadDrawer calls useProcessedTimeline without an equivalent mutation counter (it doesn't use useTimelineSync). Making the field optional with a default of 0 means: - ThreadDrawer gets stable-ref caching for free on subsequent renders (isMutation=false after first render), which is correct — it doesn't wrap items in React.memo TimelineItem. - RoomTimeline continues to pass the real mutationVersion so its TimelineItem memo components are refreshed when content mutates. - pnpm typecheck / pnpm build no longer fail with TS2345.
…mmatic scroll-to-bottom After setIsReady(true) commits, virtua can fire onScroll events with isNowAtBottom=false during its height-correction pass (particularly on first visit when item heights above the viewport haven't been rendered yet). These intermediate events were driving atBottomState to false while isReady=true, flashing the 'Jump to Latest' button. Add programmaticScrollToBottomRef: set it before each scrollToIndex bottom-scroll, suppress the first intermediate false event (clearing the guard immediately), so the next event — the corrected position or a real user scroll — is processed normally.
…ctive through all intermediate VList events Previously programmaticScrollToBottomRef was only set at a few specific call-sites and cleared after the first suppressed intermediate event. VList fires several height-correction scroll events after scrollTo(); the second one (after the clear) would call setAtBottom(false) and flash "Jump to Latest". - Move programmaticScrollToBottomRef.current = true into scrollToBottom() itself so all callers (live message arrival, timeline refresh, auto-scroll effect) are automatically guarded without missing a call-site. - Remove the guard clear in the else branch; the guard now stays active until VList explicitly confirms isNowAtBottom = true. fix(notifications): skip loadEventTimeline when event is already in live timeline When a notification tap opens a room, NotificationJumper was always navigating with the eventId URL path which triggered loadEventTimeline → roomInitialSync. If sliding sync had already delivered the event to the live timeline this produced a sparse historical slice that (a) looked like a brand-new chat and (b) left the room empty when the user navigated away and returned without the eventId. Check whether the event is in the live timeline before navigating; if it is present, open the room at the live bottom instead. Historical events still use the eventId path.
…n-empty When the app wakes from a killed state and the user taps a notification, performJump fires during the initial sync window before the room's live timeline has been populated by sliding sync. Previously, eventInLive was always false in this case, so we fell back to loadEventTimeline → roomInitialSync which loaded a sparse historical slice. On subsequent visits to the room without the eventId the room appeared empty because the live timeline was never populated for the initial roomInitialSync result. Two changes: 1. Guard: if the live timeline is completely empty, return early and wait rather than navigating — the RoomEvent.Timeline listener below retries once events start arriving. 2. Listen on RoomEvent.Timeline for the target room so performJump re-runs as soon as the first event arrives in the room, at which point the notification event is almost certainly also present so we can navigate without eventId (avoiding loadEventTimeline entirely).
…Id slice When the user taps a push notification and the app loads a historical sparse timeline via loadEventTimeline (eventId URL path), the VList item heights for those few events were being written to the room's scroll cache. On the next visit to the room (live timeline, many more events), the RoomTimeline mount read the stale cache and passed its heights to the new VList instance. The height mismatch between the sparse and live timelines caused incorrect scroll-position restoration, making the room appear to show stale or mispositioned messages. Guard the roomScrollCache.save call with !eventId so historical views never overwrite the live-timeline cache. The next live visit will either use the pre-existing (untouched) live cache or fall back to the first-visit 80 ms measurement path.
…ndex in stable-ref check
When navigating back to a previously-visited room, save the VList
CacheSnapshot (item heights) and scroll offset on the way out, then
on the way back:
• pass the snapshot as cache= to VList so item heights do not need
to be remeasured (VList is keyed by room.roomId so it gets a fresh
instance with the saved measurements)
• skip the 80 ms stabilisation timer — the measurements are already
known, so the scroll lands immediately and setIsReady(true) is
called without the artificial delay
First-visit rooms retain the existing 80 ms behaviour unchanged.
RoomTimeline mounts fresh per room (key={roomId} in RoomView), so the
render-phase room-change block used for save/load never fires.
- Init scrollCacheForRoomRef from roomScrollCache.load() on mount so the
CacheSnapshot is actually provided to VList on first render.
- Save the cache in handleVListScroll (and after the first-visit 80 ms
timer) rather than in the unreachable room-change block.
- Trim the room-change block to just the load + state-reset path (kept as
a defensive fallback for any future scenario where room prop changes
without remount).
…mmatic scroll-to-bottom After setIsReady(true) commits, virtua can fire onScroll events with isNowAtBottom=false during its height-correction pass (particularly on first visit when item heights above the viewport haven't been rendered yet). These intermediate events were driving atBottomState to false while isReady=true, flashing the 'Jump to Latest' button. Add programmaticScrollToBottomRef: set it before each scrollToIndex bottom-scroll, suppress the first intermediate false event (clearing the guard immediately), so the next event — the corrected position or a real user scroll — is processed normally.
…arator for TimelineItem memo The unused-prop-types eslint disables relied on React.memo's default shallow comparator inspecting props not read in the component body. A custom areEqual function makes the re-render triggers explicit and self-documenting.
Just-Insane
force-pushed
the
perf/timeline-item-memo
branch
from
22a4607 to
781a8e2
Compare
- Include mEvent and timelineSet in stable-ref comparison to avoid stale references after timeline reset/rebuild - Only pass VList cache snapshot for live timeline (not eventId navigations)
Keys scroll cache entries by userId:roomId so switching accounts doesn't reuse stale scroll positions from a different session.
…guard Replace the boolean programmaticScrollToBottomRef with a timestamp (ms epoch) and a SCROLL_SETTLE_MS window. The boolean approach had a race condition: VList fires isNowAtBottom=false → true → false during height re-measurement after scrollToIndex(); clearing the guard on the first 'true' event allowed the second 'false' to set atBottomState=false and flash the Jump to Latest button. The timestamp window expires naturally after 200ms without ever being reset by 'true' events, suppressing all intermediate false-negatives. Also reset atBottom and clear the guard when navigating to a specific eventId (e.g. via bookmarks) so the Jump to Latest button appears immediately when viewing a historical slice, even when the cache previously put us at the live bottom. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add programmaticScrollToBottomRef.current = Date.now() before the ResizeObserver viewport-shrink scrollTo so VList height-correction onScroll events can't flip atBottomState to false after keyboard open - Add setAtBottom(true) pre-emption inside scrollToBottom callback, preventing a one-frame flash of the Jump to Latest button and keeping atBottom=true when new-message auto-scroll fires before VList settles - Remove dead mountScrollWindowRef (declared + set, never read) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…proach Remove SCROLL_SETTLE_MS and programmaticScrollToBottomRef entirely. Use upstream's direct setAtBottom(isNowAtBottom) in handleVListScroll instead of suppressing intermediate events with a settling window. Simplify scrollToBottom to just scrollTo(scrollSize) without pre-emption. Simplify cache restore to use scrollTo(scrollSize) for atBottom rooms. Remove setAtBottom(false) from eventId effect (let scroll determine state). Add mxUserId to hook dependency arrays where used in roomScrollCache calls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use a larger threshold (300px) to lose at-bottom state vs 100px to regain it, preventing image/URL-preview layout shifts from flashing the jump button. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Guard atBottom state changes in handleVListScroll behind isReadyRef so that intermediate VList scroll events fired during the initial scrollTo() in useLayoutEffect cannot flip atBottom to false before the component has fully rendered. This prevents the 'Jump to Present' button from flashing for a single frame when opening a room where the user is already at the bottom. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 300px/100px hysteresis created a dead zone: once atBottom was lost (e.g. video embed loading causing > 300px content shift), the user could never regain it unless distanceFromBottom dropped below 100px — which VList may not report even when the scrollbar appears at the bottom. Replace with upstream's flat 100px threshold. The isReadyRef guard still suppresses initial-load flashes from scroll settling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When images, embeds, or video thumbnails load after the initial render, VList's scrollSize increases while the scroll offset stays the same. This pushed distanceFromBottom above the 100px threshold, causing atBottom to flip to false and the Jump to Latest button to appear even though the user hadn't scrolled. Track prevScrollSizeRef and detect content growth: if atBottom is true and scrollSize increased but distanceFromBottom exceeds the threshold, auto-scroll to the new bottom instead of losing atBottom status. Also removes the temporary debug logging from the previous commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…croll, fix eventId drag-to-bottom, increase list timeline limit - useTimelineSync: change auto-scroll recovery useEffect → useLayoutEffect to prevent one-frame flash after timeline reset - useTimelineSync: remove premature scrollToBottom from useLiveTimelineRefresh (operated on pre-commit DOM with stale scrollSize) - useTimelineSync: remove scrollToBottom + eventsLengthRef suppression from useLiveEventArrive; let useLayoutEffect handle scroll after React commits - RoomTimeline: init atBottomState to false when eventId is set, and reset it in the eventId useEffect, so auto-scroll doesn't drag to bottom on bookmark nav - RoomTimeline: change instant scrollToBottom to use scrollToIndex instead of scrollTo(scrollSize) — works correctly regardless of VList measurement state - slidingSync: increase DEFAULT_LIST_TIMELINE_LIMIT 1→3 to reduce empty previews when recent events are reactions/edits/state Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restore scrollToBottom call in useLiveEventArrive with instant/smooth based on sender, add back eventsLengthRef and lastScrolledAt suppression, restore scrollToBottom in useLiveTimelineRefresh when wasAtBottom, and revert instant scrollToBottom to scrollTo(scrollSize) matching upstream. The previous changes removed all scroll calls from event arrival handlers and relied solely on the useLayoutEffect auto-scroll recovery, which has timing issues with VList measurement. Upstream's pattern of scrolling in the event handler and suppressing the effect works reliably. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove behavior parameter from scrollToBottom — always use scrollTo(scrollSize) matching upstream. The smooth scrollToIndex was scrolling to stale lastIndex (before new item measured), leaving new messages below the fold. - Revert auto-scroll recovery from useLayoutEffect back to useEffect (matches upstream). useLayoutEffect fires before VList measures new items and before setAtBottom(false) in eventId effect. - Remove stale scrollCacheForRoomRef that referenced missing imports. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the isReadyRef guard to the top of handleVListScroll so that content-chase, atBottom flips, scroll-cache saves, and pagination triggers are all suppressed while the timeline is hidden (opacity 0) during VList measurement. prevScrollSizeRef is still updated so the first post-ready scroll event does not see a false content-grew delta. Previously only setAtBottom was guarded, but content-chase fired cascading scrollTo calls during the 80 ms init window — extra work that upstream does not have, producing visible layout churn when opening a room. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Compare linked-timeline references before calling setTimeline in useLiveTimelineRefresh. When the SDK fires TimelineReset during initial room load (common with sliding sync), the timeline chain is often identical — skipping the update avoids a full re-render flash. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a genuine TimelineReset replaces the timeline chain (new EventTimeline objects), hide the timeline behind opacity 0 and re-arm the initial-scroll mechanism. This ensures the replacement data renders invisibly, gets measured by VList, and only becomes visible once stable — preventing the visible flash/jitter on room open. Adds timelineResetToken counter to useTimelineSync that increments on genuine resets. RoomTimeline watches it via useLayoutEffect to toggle isReady off before the browser paints the intermediate state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When opening a room with no cached scroll state, render a skeleton placeholder overlay on top of the VList while it measures real item heights at opacity 0. This gives users immediate visual feedback instead of a blank/invisible area during the 80ms stabilisation window. Cached rooms skip the overlay entirely since they restore instantly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Wraps each VList timeline item in
React.memoso that only the items whose props actually changed re-render when state updates occur (e.g. typing indicators arriving, read receipts updating, or new messages landing while the user is scrolled away from the bottom).Props passed to each item beyond
dataandrenderRef—isHighlighted,isEditing,isReplying,isOpenThread,settingsEpoch— exist solely to giveReact.memo's shallow-equality comparator fine-grained control: changing any of them causes only the one affected item to re-render rather than the entire visible list.Also makes
mutationVersionoptional (default0) inUseProcessedTimelineOptionsso call sites that don't participate in the mutation tracking (e.g.ThreadDrawer) continue to compile without changes.Fixes #
Type of change
Checklist:
AI disclosure:
useProcessedTimeline.ts— added astableRefsCache(auseRef<Map<string, ProcessedEvent>>) that keeps the previous render'sProcessedEventobjects alive. On eachuseMemorun, ifmutationVersionhasn't changed, events whose identity hasn't changed are returned from the cache (same object reference) rather than newly constructed. This meansReact.memosees no prop change for stable items and skips their render.RoomTimeline.tsx—TimelineItemis aReact.memo-wrapped component declared outside the parent function so it is never re-created. ArenderFnRefref captures the render function soTimelineItemitself only needs a stable ref in props, not the function.settingsEpochis auseRef({}).currentthat is replaced with{}whenever any layout/display setting changes, forcing all visible items to pick up the new settings in one pass.